Implement `TryInto` for `Value` #947

phayes · 2022-11-02T19:53:16Z

Fixes #902

This PR implements TryInto for converting Value into common types such as String, str, u64, f64, i64, (), Vec and Map. I've implemented these both for owned Value and borrowed Value.

Justification and Use Case

I know there's been some skepticism from the maintainer about the need for TryInto impls when one can use the full deserialization machinery to retrieve a concrete typed value from a Value, but I'd like to justify this PR by pointing to both the ergonomics of having TryInto and the efficiency of TryInto over full deserialization (especially when borrowing).

For my use-case, I have a struct that delays fully deserializing all values until much later in the program.

struct Job {
    pub name: String,
    pub inputs: HashMap<String, Value>
}

Much deeper in the code, inputs get wrapped into a container like so:

pub struct Inputs {
    inner: HashMap<String, Value>
}

impl Inputs {

    pub fn get_required<'a, T: ?Sized>(&'a self, key: &str) -> Result<&'a T, Error>
        where &'a Value: TryInto<&'a T>
    {
        match self.inner.get(key) {
            Some(value) => {
                value.try_into()?
            },
            None => Err(Error)
        }
    }

    pub fn get_with_default<'a, 'default: 'a, T: ?Sized>(&'a self, key: &str, default: &'default T) -> Result<&'a T, Error> 
        where &'a Value: TryInto<&'a T> 
    {
        match self.inner.get(key) {
            Some(value) => {
                value.try_into()?
            },
            None => {
                Ok(default)
            }
        }
    }
}

All of this machinery provides a really nice interface to calling code, that can simply call into Inputs like so:

let foo: &str = inputs.get_required("foo")?;
let bar: &str = inputs.get_with_default("bar", "baz")?;

And importantly, because inputs can be quite large, everything is done via borrows, avoiding unnecessary cloning and allocations.

This is my specific use-case, but I'm sure there are other use-cases where having reasonable and straightforward TryInto impls for Value would be ergonomic and timesaving.

Thank you.

kangalio · 2023-05-23T17:54:56Z

TryInto shouldn't be implemented explicitly, you should implement TryFrom instead. See https://doc.rust-lang.org/std/convert/trait.TryInto.html#implementing-tryinto

Also, TryInto/TryFrom in general is the wrong choice here. Similarly to From/Into, TryFrom/TryInto should be used to convert between types that are semantically equivalent, but differ only in programmatical representation. For example, std implements TryFrom to convert integer<->integer and list<->list.

Instead, there should be into_xxx() methods on Value, like the existing as_xxx() methods.

SabrinaJewson · 2023-05-23T18:02:25Z

I don’t see why you can’t just use T: Deserialize in the code example you posted. &Value is a Deserializer so it should just work.

Edit: Nevermind, I misread. Even so, it does feel like the kind of thing that should be done strongly-typed to begin with.

phayes · 2023-05-26T20:32:06Z

Edit: Nevermind, I misread. Even so, it does feel like the kind of thing that should be done strongly-typed to begin with.

Strong typing is definitely preferred all-else-equal, but in this particular case it really does make more sense to keep it loosely typed until later in the program. For my use-case, we don't have enough information up-front to know what the shape of the data should be. Once we know what the shape should be, then we use more well-defined types.

forrestli74 · 2023-07-08T04:29:45Z

Also, TryInto/TryFrom in general is the wrong choice here. Similarly to From/Into, TryFrom/TryInto should be used to convert between types that are semantically equivalent, but differ only in programmatical representation. For example, std implements TryFrom to convert integer<->integer and list<->list.

FWIW, using u64 as an example, we already have Value implementing TryFrom, which breaks this principle. I think it's only fair to stay consistent. (Which is to either add TryInto or remove TryFrom).

phayes added 2 commits November 2, 2022 12:18

TryInto impls for Value

f7f18cf

Make sure use of alloc is explicit in TryInto impls for Value

55ff89b

This comment was marked as spam.

Sign in to view

kangalio mentioned this pull request May 23, 2023

Convert Value to String in one line #834

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `TryInto` for `Value` #947

Implement `TryInto` for `Value` #947

phayes commented Nov 2, 2022 •

edited

Loading

This comment was marked as spam.

kangalio commented May 23, 2023

SabrinaJewson commented May 23, 2023 •

edited

Loading

phayes commented May 26, 2023 •

edited

Loading

forrestli74 commented Jul 8, 2023

Implement TryInto for Value #947

Are you sure you want to change the base?

Implement TryInto for Value #947

Conversation

phayes commented Nov 2, 2022 • edited Loading

Justification and Use Case

This comment was marked as spam.

kangalio commented May 23, 2023

SabrinaJewson commented May 23, 2023 • edited Loading

phayes commented May 26, 2023 • edited Loading

forrestli74 commented Jul 8, 2023

Implement `TryInto` for `Value` #947

Implement `TryInto` for `Value` #947

phayes commented Nov 2, 2022 •

edited

Loading

SabrinaJewson commented May 23, 2023 •

edited

Loading

phayes commented May 26, 2023 •

edited

Loading